Skip to content

testintel: add missing test for AvgDown3D.forward (created by linxwang) - #1096

Merged
jiagaoxiang merged 6 commits into
mainfrom
testintel/test/cand-9cf47afdeacb6b6d0378129eb4bf01c2
Sep 7, 2026
Merged

testintel: add missing test for AvgDown3D.forward (created by linxwang)#1096
jiagaoxiang merged 6 commits into
mainfrom
testintel/test/cand-9cf47afdeacb6b6d0378129eb4bf01c2

Conversation

@jiagaoxiang

@jiagaoxiang jiagaoxiang commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator

Same-repo refile of #1083 so Primus-CI-TAS runs against AMD-AGI/Primus (fork PRs do not receive Docker Hub credentials, so build-docker / torch unit tests never ran).

This PR was created by linxwang via Test Gap Resolver.

No existing test in tests/unit_tests/backends/diffusion (or anywhere else in the repo) instantiates AvgDown3D or imports vae2_2.py at all; the pad->view->permute->view->view->mean sequence that implements grouped spatiotemporal average-downsampling, including the temporal padding branch (pad_t>0) and the group_size averaging for group_size>1 with in_channels != out_channels (plus the independent spatial/channel mapping oracle), is completely unexercised.

This PR adds a focused unit test for AvgDown3D.forward; it does not change production code.

TestIntel and others added 3 commits September 3, 2026 01:38
Candidate cand:9cf47afdeacb6b6d0378129eb4bf01c2 at 2f01706.
…rence

Address review: compare full output against a hand-computed reference for
factor_s=2 with in_channels != out_channels (deterministic torch.arange
input), verifying spatial block grouping and channel/group mapping instead
of only asserting output shape.
pre-commit's black hook wraps the AvgDown3D(...) constructor call in
test_forward_downsamples_spatial_and_channel_dims onto multiple lines
(line was too long). No behavior change.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The change is isolated to a new unit test file and the assertions align with the current AvgDown3D implementation behavior.

Pull request overview

Adds targeted unit-test coverage for the WAN VAE AvgDown3D downsampling module to ensure its grouped channel averaging and temporal/spatial downsampling behaviors are exercised in CI.

Changes:

  • Introduces a new unit test module that instantiates AvgDown3D and validates output shape/value semantics.
  • Covers both the temporal left-padding branch (pad_t > 0) and the grouped-channel averaging path (group_size behavior).
  • Adds a negative test to confirm incompatible (in_channels * factor) % out_channels != 0 is rejected.
File summaries
File Description
tests/unit_tests/backends/diffusion/test_wan_vae_avgdown3d.py New focused tests validating AvgDown3D.forward behavior (padding, reshape/permute mapping, averaging) plus init assertion.
Review details
  • Files reviewed: 1/1 changed files
  • Comments generated: 0
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@WangLingxun

Copy link
Copy Markdown
Collaborator

The revised test itself looks good and passes against current main when overlaid locally. One small PR-description correction: in_channels * factor != out_channels * group_size cannot be the covered condition because group_size is defined so those quantities are equal. The non-trivial case covered here is group_size > 1 with in_channels != out_channels, plus the independent spatial/channel mapping oracle. Please adjust that sentence; no test-code change is needed.

@jiagaoxiang

Copy link
Copy Markdown
Collaborator Author

TestIntel PR Steward: Thanks @WangLingxun for the catch. You're right that in_channels * factor and out_channels * group_size are equal by construction of group_size, so that condition can't be what's covered. I've updated the PR description to say the non-trivial case exercised here is group_size > 1 with in_channels != out_channels, plus the independent spatial/channel mapping oracle. No test-code change was needed, per your note.

Copilot's automated review came back with an approval recommendation and no actionable comments. This PR remains draft; I'll keep watching for CI, further review comments, and a human approval before merging.

@@ -0,0 +1,94 @@
###############################################################################
# Copyright (c) 2025, Advanced Micro Devices, Inc.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a newly added file, so the copyright year should be 2026.

Address review comment from WangLingxun: newly added file should use 2026.
Copilot AI review requested due to automatic review settings September 7, 2026 04:25
@jiagaoxiang

Copy link
Copy Markdown
Collaborator Author

TestIntel PR Steward: Addressing @WangLingxun's review comment on tests/unit_tests/backends/diffusion/test_wan_vae_avgdown3d.py line 2: updated the copyright header from 2025 to 2026 since this is a newly added file. Pushed as commit 25bde0a (new head SHA 25bde0a06ee3ba09785b41a2003d1e3d7841e1bb).

No other outstanding review comments. Still draft; watching for CI, further review, and a human approval before requesting a merge.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The change is test-only, aligns with the stated goal, and the assertions match the current AvgDown3D implementation (padding and reshape/permute/group-mean behavior).

Review details
  • Files reviewed: 1/1 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

Copilot AI review requested due to automatic review settings September 7, 2026 06:06

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review is ineligible. To be eligible to request a review, you need a paid Copilot license, or your organization must enable Copilot code review.

@WangLingxun
WangLingxun marked this pull request as ready for review September 7, 2026 06:07
@jiagaoxiang

Copy link
Copy Markdown
Collaborator Author

TestIntel PR Steward: Investigated the CI failure on the current head (f6f662c, @WangLingxun's merge of main into this branch).

Most of the checks the poll flagged as failed (build-docker, code-lint, dependency-review, run-unittest-jax) belong to an earlier, superseded workflow run (34089417005) that GitHub cancelled when the merge commit triggered a fresh run — those are stale cancellations, not real failures.

The one genuine failure, in the latest run (34089420822, run-unittest-torch), is tests/trainer/test_megatron_trainer.py::TestProjectionSimulate::test_memory_benchmark erroring with:

torch.distributed.DistNetworkError: The server socket has failed to listen on any local network address. port: 53395, ... EADDRINUSE, message: address already in use

That's an unrelated test (Megatron trainer memory benchmark, not AvgDown3D) hitting a leftover/occupied rendezvous port on the runner — a classic infra flake, not a defect introduced by this PR's single new test file. I've requeued the failed jobs on that run and will watch for green.

Copilot AI review requested due to automatic review settings September 7, 2026 09:53
@jiagaoxiang

Copy link
Copy Markdown
Collaborator Author

TestIntel PR Steward: Status check on this poll:

CI is now running on the new head via run 34108505438. I'll merge once that run is green, provided the approval still stands and no one asks to hold.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

It adds well-scoped, deterministic unit tests that match the implementation’s padding and reshape/permute semantics without changing production behavior.

Review details
  • Files reviewed: 1/1 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

@jiagaoxiang
jiagaoxiang merged commit 2f87a4b into main Sep 7, 2026
10 checks passed
@jiagaoxiang

Copy link
Copy Markdown
Collaborator Author

TestIntel PR Steward: Merged (squash, 2f87a4b).

All conditions were met at this poll:

  • CI: all 6 jobs on Primus-CI-TAS run 34108505438 green on the current head (9844593) — dependency-review, code-lint, build-docker, run-unittest-jax, run-unittest-torch, coverage-summary — plus the Read the Docs check succeeded.
  • Review: @Xiaoming-AMD's human approval stands; @WangLingxun's copyright-year comment was addressed in 25bde0a and their PR-description correction was applied; no outstanding human change requests or hold/WIP comments.
  • Mergeable state: clean, no conflicts.

Thanks all for the review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants